Blocks: Share window listeners across instances (block props, rich text, ...)#78310
Merged
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ellatrix
force-pushed
the
perf/share-rich-text-document-listeners
branch
from
a5234d3 to
889189c
Compare
|
Size Change: +661 B (+0.01%) Total Size: 7.98 MB 📦 View Changed
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
perf/share-rich-text-document-listeners
branch
from
889189c to
c0de9b8
Compare
|
Flaky tests detected in aa309f0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/26174943358
|
ellatrix
changed the title
Each RichText hook setup adds four `addEventListener` calls to the shared `defaultView` (`copy` / `cut` from copy-handler, `pointerdown` / `pointerup` from prevent-focus-capture). Every handler short-circuits when the rich-text isn't the focused one, so N listeners do the work of 1 — the rest are pure JS↔C++ boundary crossings on mount. With many rich-text instances mounted at once (paragraphs, headings, buttons, lists in a large post each contain a RichText), the boundary-crossing cost is the largest remaining source of `addEventListener` time during the React commit — ~50 ms total in a representative post-editor first-block trace, of which ~10 ms is the shared-target subset this change addresses. Introduce `subscribeSharedListener( target, eventType, callback )` in `packages/rich-text/src/hook/event-listeners/shared-listener.js`: keeps a `WeakMap<EventTarget, Map<eventType, Set<callback>>>` and attaches one native listener per (target, type) that fans out to every subscriber. Refactor `copy-handler` and `prevent-focus-capture` to use it. Same handler semantics, same call order, no API change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…handlers Both `enter` (block-editor/rich-text/event-listeners/enter.js) and `paste-handler` (same dir) attach their listeners to `defaultView` so parent elements can intercept first. Each handler then checks `event.target === element` (or `element.contains(event.target)`) and short-circuits if the rich-text isn't the focused one — same pattern as `copy-handler` and `prevent-focus-capture`. Route both through the shared registry so all RichText instances share one native `keydown` / `paste` listener on the window instead of one each. Expose `subscribeSharedListener` from `@wordpress/rich-text`'s private APIs so block-editor can use the same helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move `delete.js`'s `keydown` listener off the contenteditable and onto `ownerDocument` via `subscribeSharedListener`. The handler now bails if our editable isn't the focused element, mirroring the `copy-handler` / `prevent-focus-capture` pattern. Behaviour for Backspace/Delete-of-all-selected-content is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move `format-boundaries.js`'s `keydown` listener off the contenteditable and onto `ownerDocument` via `subscribeSharedListener`, following the same pattern as `delete.js`. The handler bails when our editable isn't the focused element. Behaviour for LEFT/RIGHT at format boundaries is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`use-arrow-nav.js` in block-editor gates on `event.defaultPrevented` and assumes rich-text's `format-boundaries` keydown handler has already run. With the shared listener on `ownerDocument` in bubble phase, that ordering reversed: ancestor element listeners fired before the document-level handler. Extend `subscribeSharedListener` with a capture-phase option and switch `format-boundaries` to use it. The handler now fires before any ancestor bubble listeners up the chain, restoring the original `defaultPrevented` contract. Fixes the inline-boundary navigation regressions in `writing-flow.spec.js` and `rtl.spec.js`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move `select-object.js`'s `click` and `focusin` listeners off the contenteditable and onto `ownerDocument` via `subscribeSharedListener`, in bubble phase. Add an explicit `! element.contains( event.target )` guard at the top of `onClick` — the previous implicit guard only worked because the listener was element-scoped. `onFocusIn` is unchanged; its delegation to `onClick` is filtered by the new guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…change compat Move `selection-change-compat.js`'s `pointerdown` and `keydown` listeners off the contenteditable and onto `ownerDocument` via `subscribeSharedListener` (bubble phase). Add an `! element.contains( event.target )` guard at the top of `onDown` so only the matching instance schedules its transient up/cancel listeners and dispatches the synthetic `selectionchange`. The transient inner listeners (`pointerup`/`keyup`, `selectionchange`, `input`) are unchanged — they are per-event, not per-mount, and self-cancel. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move `input-and-selection.js`'s `input`, `compositionstart`,
`compositionend`, and `focus` listeners off the contenteditable and
onto `ownerDocument` via `subscribeSharedListener` (bubble phase).
- `input` / `compositionstart` / `compositionend`: guarded by
`! element.contains( event.target ) → return`. The `onInput` guard
is conditional because `onCompositionEnd` synthetically invokes
`onInput({ inputType: 'insertText' })` with no real event.
- `focus`: switched to `focusin` since `focus` doesn't bubble; guarded
by `event.target !== element` to preserve strict element-level
`focus` semantics (a focusable descendant getting focus didn't
trigger the old listener).
This was the last per-instance element-level listener in the rich-text
hook. All seven event-listener modules now share their native
listeners across rich-text instances.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eners `block-editor`'s `input-rules.js` and `input-events.js` attach element-level `input` / `compositionend` listeners and read `getValue()` (which reads `record.current`). Rich-text's own `onInput` must run first to update `record.current`, otherwise input-rules sees stale text and trigger patterns (`>` → quote, `\`` → code, `---` → separator) stop matching. Semantically this is the right phase: `input` fires after the browser has mutated the DOM, and rich-text's job is to sync its internal record to that mutation before any consumer reads it. Capture-phase expresses "I establish baseline state for this event." Fixes the quote / backtick / separator transformation regressions in the e2e suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A locked binding paragraph has `contentEditable="false"` but still `tabindex=0`, so it can receive focus. With rich-text's `onFocus` now on a shared `focusin` listener it ran on doc-bubble — after block-editor's element-level `use-focus-handler.js`. The latter already dispatches `selectionChange(clientId)` (no `attributeKey`) for focus on a non-editable target, but our handler then ran second and re-set `attributeKey: 'content'`, leaving the rich text "selected for editing" and the inline format toolbar (Bold, Italic, Link) visible. Mirror the existing guard in `handleSelectionChange` (line 120) and bail in `onFocus` when `contentEditable !== 'true'`. The rich text shouldn't claim selection when it isn't actually editable. Fixes the block-bindings "should lock editing" regressions in custom-sources.spec.js. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migrate `useFocusHandler`'s per-block `focusin` listener to a shared `subscribeSharedListener( node.ownerDocument, 'focusin', … )` with a `! node.contains( event.target )` bail at the top. Collapses ~N native focusin listeners (one per mounted block) into one shared document listener. Subscription ordering is preserved: rich-text's `useEventListeners` is composed inside `useRichText`, whose `useRefEffect` runs before the block wrapper's `useRefEffect` in the ref chain (see `block-editor/src/components/rich-text/index.js:427-457`). So per focusin, rich-text's onFocus fires first and use-focus-handler fires second — same order as trunk (`focus` → `focusin`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migrate `useIsHovered`'s per-block `mouseover` / `mouseout` listeners to a shared `subscribeSharedListener( node.ownerDocument, …, bubble )` with a `! node.contains( event.target )` bail. For each event, only the subscribers whose wrapper contains the target toggle their own `is-hovered` class — preserves the existing nested-block behavior (both Paragraph and parent Group get the class when hovering a child of Paragraph). Collapses ~2×N native listeners (one mouseover + one mouseout per hover-enabled block) into 2 shared document listeners. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move the `subscribeSharedListener` helper from `packages/rich-text/src/hook/event-listeners/shared-listener.js` to `@wordpress/compose` as a public utility. Lower-layer packages such as `@wordpress/components` can now consume it without depending on `@wordpress/rich-text` (which would be a layer reversal). All seven rich-text event-listener modules and the four block-editor consumers (`enter`, `paste-handler`, `use-focus-handler`, `use-is-hovered`) now import the helper from `@wordpress/compose` directly. The helper is no longer re-exported from rich-text's private API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The helper was added as a `.js` file but the compose `tsconfig` has `checkJs: true` + `strict`, which flagged the implicit `any` parameter types. Convert to `.ts` with explicit types: `target: EventTarget`, `eventType: string`, `callback: ( event: Event ) => void`, `capture: boolean = false`. No runtime change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migrate `useAutocompleteProps`'s per-instance `keydown` listener to a shared `subscribeSharedListener( ownerDocument, 'keydown', …, capture )` with a `! element.contains( event.target )` bail. Collapses ~N native keydown listeners (one per RichText) into one shared document capture-phase listener. Capture phase: when the autocomplete popover is open, Up/Down/Enter/Escape must navigate the completion list — they shouldn't be consumed by ancestor handlers (e.g. block-editor's writing-flow) for block navigation, block splitting, or "move out of parent" actions. Those handlers fire at bubble phase and gate on `event.defaultPrevented`, so firing in capture lets us `preventDefault` first when the popover is active. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`subscribeSharedListener`'s `Listener` type is `(event: Event) => void`, but the previous commit passed a `(event: KeyboardEvent) => void` callback. With strict mode, function parameter types are contravariant, so the narrower signature isn't assignable. Widen `_onKeyDown` to `(event: Event) => void` and cast to `KeyboardEvent` when delegating to `onKeyDownRef.current`. The shape of the keydown event at runtime is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Migrate the per-block-on-mount `keydown` listeners in: - `paragraph/use-enter.js` - `list-item/hooks/use-enter.js` - `list-item/hooks/use-space.js` - `button/edit.js` (the local `useEnter`) to a shared `subscribeSharedListener( ownerDocument, 'keydown', …, capture )` with a `! element.contains( event.target )` bail. Each collapses ~N native listeners (one per mounted paragraph / list item / button) into one shared document capture-phase subscription. Capture phase preserves the existing ordering contract: on trunk these listeners ran at target phase on the inner block element, so they fired before writing-flow's ancestor-bubble handlers (`use-input.js`, `use-arrow-nav.js`), which gate on `event.defaultPrevented` to defer to the more specific consumer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ellatrix
force-pushed
the
perf/share-rich-text-document-listeners
branch
from
b312e8f to
37bf7c6
Compare
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ellatrix
force-pushed
the
perf/share-rich-text-document-listeners
branch
from
37bf7c6 to
6e208e4
Compare
Replace `! element.contains( event.target )` with `event.target !== element` (and `! element.contains( activeElement )` with the event.target check) in the shared subscribers whose events fire on the focused leaf element: - `rich-text/delete.js` - `rich-text/format-boundaries.js` - `rich-text/input-and-selection.js` (input, compositionstart, compositionend) - `paragraph/use-enter.js` - `list-item/use-enter.js` - `list-item/use-space.js` - `button/edit.js` useEnter - `components/Autocomplete` Keydown/input/composition fire on the focused element directly — for the leaf contenteditable that hosts these handlers there are no focusable descendants in core blocks, so `target === element` is sufficient and cheaper than a DOM walk. `select-object.js`, `selection-change-compat.js`, `use-focus-handler.js`, and `use-is-hovered.js` keep `contains()` — their events (click, pointerdown, focusin, mouseover/mouseout) can legitimately fire on descendants (`<strong>` in formatted text, focusable children of a block wrapper, etc.). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jsnajdr
reviewed
May 20, 2026
| // inside this block's wrapper. | ||
| if ( ! node.contains( event.target ) ) { | ||
| return; | ||
| } |
Member
There was a problem hiding this comment.
I'd expect the node check to be inside subscribeSharedListener. The React event system has only one document listener and then calls individual node listeners according to event.target. Our version calls all listeners on all events, even when they happen in a different element.
A better API would be:
subscribeSharedListener( node, 'focusin', onFocus );where the onFocus handler is called only when the event really happens inside node, and the helper can figure out node.ownerDocument from the node.
Member
Author
There was a problem hiding this comment.
Yes, it's a good idea. Adjusted 🙂
Per @jsnajdr's review: instead of every callback running on every event with a manual `! element.contains( event.target )` bail, let the helper accept the bound element directly and dispatch via a target → ancestors walk — only callbacks bound to a node on the event path get invoked. Mirrors how React's synthetic event system works. The helper now accepts `Element | Document | Window` as the target: - `Element`: attaches one native listener at `element.ownerDocument`, maintains a per-element registry, dispatches via DOM ancestry. Capture phase walks root-to-target; bubble walks target-to-root. - `Document`: always dispatches (document is the root of every event in it, so it's naturally on every ancestry path). - `Window`: flat fan-out (window isn't on the DOM tree). All element-scoped call sites switched from passing `element.ownerDocument` to passing `element` directly, and drop their own `! element.contains( event.target )` / `event.target !== element` guard at the top of the callback. Window-scoped subscriptions (`copy-handler`, `prevent-focus-capture`, block-editor `enter` / `paste-handler`) are unchanged — they intentionally observe events outside the rich-text. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`instanceof Document` returns false when the target is a document from a different realm — e.g. the editor canvas iframe's document, whose constructor is iframe.contentWindow.Document, not the parent window's Document. The check fell through to `target.ownerDocument`, which is `null` for a Document, then `registries.set(null, …)` threw "Invalid value used as weak map key" during rich-text mount inside the canvas iframe. Switch to duck-typing checks that work across realms: - `target.nodeType === 9` (DOCUMENT_NODE) for Document - `target.window === target` for Window Also bundles the bubble-phase inline dispatch optimisation from the walk benchmark (no path-array allocation for the common case). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`subscribeSharedListener` is not intended for use in themes or plugins — it relies on subscribers cooperating about phase ordering and target scoping, and the wrong call shape can corrupt the shared registry. Move it from a public export to `privateApis` via the standard `__dangerousOptInToUnstableAPIsOnlyForCoreModules` pattern. Introduces private-API plumbing in compose for the first time: - New `packages/compose/src/lock-unlock.ts` - New `packages/compose/src/private-apis.ts` (exports `privateApis` with `subscribeSharedListener` locked inside) - `@wordpress/private-apis` added as a compose dependency - Public export removed from `packages/compose/src/index.js` All 16 consumers (7 in rich-text, 4 in block-editor, 4 in block-library, 1 in components) updated to `unlock( composePrivateApis )`. Behaviour unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Required since the previous commit moved `subscribeSharedListener` behind compose's private API. Without this entry the allowlist throws during compose's `__dangerousOptInToUnstableAPIsOnlyForCoreModules` call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Validate-tsconfig requires explicit project references for each package dependency. Added when introducing compose's privateApis but the tsconfig wasn't updated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Missed running `npm install` after adding the dependency in the "Make subscribeSharedListener private" commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jsnajdr
approved these changes
May 20, 2026
| import { store as blockEditorStore } from '../../../store'; | ||
| import { unlock } from '../../../lock-unlock'; | ||
|
|
||
| const { subscribeSharedListener } = unlock( composePrivateApis ); |
Member
There was a problem hiding this comment.
I wouldn't be afraid to make it public immediately. It's a solid API and there are no doubts about its usefulness.
One little suggestion: instead of the capture param, support options = { capture } object. Or both, the goal is symmetry with addEventListener.
Member
Author
There was a problem hiding this comment.
The only thing I'm not sure of is the naming 😆
Member
There was a problem hiding this comment.
I was thinking about subscribeDocumentListener, but that doesn't work very well for window 🙂
Member
There was a problem hiding this comment.
It shouldn't be "add" because it doesn't behave like addEventListener. It returns an unsubscribe callback, there is no "remove" function like removeEventListener. So, the "subscribe" part is right 🙂 It's about the adjective: "shared listener", "document listener"?
I asked Cursor (inside React repo context, where this pattern is used for the entire event system) and it says firmly it should be subscribeDelegatedListener, insisting that "event delegation" is a standard established name for what we're doing.
Member
Author
There was a problem hiding this comment.
I like subscribeDelegatedListener
Member
Author
There was a problem hiding this comment.
I renamed, but I'll leave it private for now. It's always easy to make public.
jsnajdr
reviewed
May 20, 2026
| } | ||
| set.add( callback ); | ||
| return () => { | ||
| set?.delete( callback ); |
Member
There was a problem hiding this comment.
nit: the optional chaining is not needed, set is always valid. set.add is called one line before and the variable is not going to disappear.
Member
There was a problem hiding this comment.
It's also weird that the listener is never removed. It becomes a noop when the subscribers set becomes empty, but stays there forever.
Member
Author
There was a problem hiding this comment.
Right, but it's one listener by event type per document, so not sure if there's much value. Your comment actually made me realize it wasn't using a weak map, so added that now. That should take care of iframes being deleted
jsnajdr
reviewed
May 20, 2026
| * to `false`. | ||
| * @return Unsubscribe function. | ||
| */ | ||
| type Listener = ( event: Event ) => void; |
Member
There was a problem hiding this comment.
There's native EventListener type for this, similar to EventTarget that we already use.
Per review feedback from @jsnajdr, address two leaks/nits: 1. **WeakMap inner registry** — Element subscribers are now held weakly so an iframe removal lets the iframe's Elements (and through them, `ownerDocument`) be garbage-collected. Without this, the registry kept Element keys strong, and Element keys hold `ownerDocument` strong, so even after an iframe is yanked out of the DOM the document would stay alive (and so would the native listener attached to it). The native listener is attached to the document itself, so it goes when the document goes — no explicit `removeEventListener` needed. Window fan-out is updated to fetch the single window-keyed Set via `subscribers.get( root )` rather than iterating `.values()` — all window-bound subscribers share the same key (the window itself), so one lookup suffices and WeakMap doesn't need to be iterable. 2. **Use the native `EventListener` type** rather than rolling our own `Listener` alias. 3. **Drop defensive optional chaining** on `set?.delete()` — `set` is guaranteed to exist (`set.add` ran on the line above). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review feedback: "delegated" describes what the helper does (event delegation) rather than how it's implemented. Renames the function, its module directory, and all 16 consumer call sites. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Collapse ~19 per-block native event listeners (14 per RichText + 3 per block wrapper + 1 from block-library + 1 from
Autocomplete) into shared document listeners via a newsubscribeSharedListenerhelper in@wordpress/compose.Why?
On a large post, the editor was registering ~26k native event listeners on mount. The C++↔JS boundary crossings show up as a wide band of
addEventListenercalls during load. React already does this for synthetic events (one delegated listener at the root); this PR applies the same pattern to the editor'suseRefEffectlisteners that React doesn't cover.How?
subscribeSharedListener( target, eventType, callback, capture )in@wordpress/compose— lazily registers one native listener per(target, eventType, phase)and fans out to subscribers via an in-JSSet. Capture-phase variant for handlers that need to fire before ancestor bubble listeners gating onevent.defaultPrevented.@wordpress/rich-text) — all seven event-listener modules (copy-handler,prevent-focus-capture,delete,format-boundaries,select-object,selection-change-compat,input-and-selection) migrated. Element-level listeners gone; subscribers join the sharedSetand bail on! element.contains( event.target )(or equivalent).@wordpress/block-editor) —use-focus-handler,use-is-hovered, and the rich-text wrappersenter/paste-handlermigrated.@wordpress/block-library) —paragraph/use-enter,list-item/use-enter,list-item/use-space,buttonuse-enter migrated (capture phase to preserve the trunk ordering against writing-flow'sdefaultPreventedgate).@wordpress/components) —Autocomplete's per-instancekeydownlistener migrated (capture phase).Testing Instructions
Screenshots
26k → 2k event listeners (large document)
They also show up during editor mount — all the gold
addEventListenercalls in this window:Use of AI Tools
Authored with Claude Code assistance; reviewed and verified by the author.